Skip to content

Improve dependency management between buildkite steps #4803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 12 commits into from
Sep 16, 2024
Merged

Improve dependency management between buildkite steps #4803

merged 12 commits into from
Sep 16, 2024

Conversation

roypat
Copy link
Contributor

@roypat roypat commented Sep 13, 2024 •
edited
Loading

Changes

  • Make the build group not block on the shared compilation step (by moving the 2 tests that require the shared artifacts to the functional group)
  • Make x86 functional tests not wait on arm compilation finishing

Reason

my ocd, mostly

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this
    PR.
  • API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • New TODOs link to an issue.
  • Commits meet
    contribution quality standards.
  • This functionality cannot be added in rust-vmm.

@roypat
move test_licences from test_dependencies.py to test_licences.py
328894a 
`test_dependencies.py` only contained a single test, which tested the
licences of the dependencies. It only ran on Intel. Thus, it was a style
check in all but name. Move it to `test_licences.py`, which is where it
belongs.

Signed-off-by: Patrick Roy <[email protected]>
@roypat
test: remove dead code from test_licences.py
e9f6f7d 
The `return True` was flagged by IntelliJ as unreachable (and I concur),
the main function was probably to allow running the test without pytest,
but we have separate mechanism for this these days (`./tools/devtool
checkstyle`).

Signed-off-by: Patrick Roy <[email protected]>
@roypat
test: move test_binary_{size,static_linking}.py to functional tests
175fe78 
The idea here is that these tests depend on the compilation step, and
thus test a production binary. This smells "integration" to me. The
actual reason for moving this however is so that we can have the `build`
group no longer depend on the shared compilation step.

Signed-off-by: Patrick Roy <[email protected]>
@roypat
buildkite: eliminate initial_steps
bd1391f 
The point of these is to run in parallel with the compilation steps, and
not block the wait step if they fail. However, since we use `depends_on:
null` for these, it doesn't matter whether they are initial or not, so
just add them via `add_step` with `decorate=False`, as we do for normal
steps.

Signed-off-by: Patrick Roy <[email protected]>
@roypat
buildkite: Do not block build group on compilation
4a5fdde 
No tests in the build group use the pre-compiled binaries, as these
tests only run clippy, compute code coverage, and run unittests. So just
start running these immediately.

Signed-off-by: Patrick Roy <[email protected]>
@roypat
refactor: buildkite: store architecture per instance
9553c0a 
Store whether a given instances is x86 or arm

Signed-off-by: Patrick Roy <[email protected]>
@roypat roypat force-pushed the buildkite-dependencies branch from 291ac04 to 47dfba2 Compare September 13, 2024 16:33
@roypat
refactor: buildkite: Use build_group_per_arch for shared build
2f08edb 
Reduces code duplication slightly

Signed-off-by: Patrick Roy <[email protected]>
@roypat roypat force-pushed the buildkite-dependencies branch from 47dfba2 to f0fa22e Compare September 13, 2024 16:36
@codecov Codecov
Copy link

codecov bot commented Sep 13, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.32%. Comparing base (ec7e13a) to head (8dbca10).
Report is 11 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #4803   +/-   ##
=======================================
  Coverage   84.32%   84.32%           
=======================================
  Files         249      249           
  Lines       27521    27521           
=======================================
  Hits        23206    23206           
  Misses       4315     4315
Flag Coverage Δ
5.10-c5n.metal 84.54% <ø> (ø)
5.10-m5n.metal 84.52% <ø> (ø)
5.10-m6a.metal 83.81% <ø> (ø)
5.10-m6g.metal 80.89% <ø> (ø)
5.10-m6i.metal 84.51% <ø> (ø)
5.10-m7g.metal 80.89% <ø> (ø)
6.1-c5n.metal 84.54% <ø> (+<0.01%) ⬆️
6.1-m5n.metal 84.52% <ø> (ø)
6.1-m6a.metal 83.82% <ø> (+<0.01%) ⬆️
6.1-m6g.metal 80.89% <ø> (ø)
6.1-m6i.metal 84.51% <ø> (ø)
6.1-m7g.metal 80.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@roypat roypat force-pushed the buildkite-dependencies branch from f0fa22e to 2c05520 Compare September 13, 2024 16:51
pb8o
pb8o reviewed Sep 16, 2024
View reviewed changes
@pb8o pb8o mentioned this pull request Sep 16, 2024
Merged
9 tasks
@roypat
buildkite: generate keys for per-arch steps
71ffdbe 
These will allow us to have x86 tests only block on x86 compilation, and
arm tests only on arm compilation.

Signed-off-by: Patrick Roy <[email protected]>
@roypat
buildkite: Replace wait step with explicit dependencies
1dfcab7 
This way, if the x86 build is faster than the ARM build, the x86 tests
can start running while the ARM ones are still blocked.

Signed-off-by: Patrick Roy <[email protected]>
@roypat
refactor: buildkite: rename decorate to depends_on_build
dd1af79 
Name more clearly indicate what the parameter does (adding `depends_on`
steps for the shared compilation, and adapting the step commands with an
artifact download).

Signed-off-by: Patrick Roy <[email protected]>
@roypat
buildkite: add comment about kwargs consumed by build_group_per_arch
353e8af 
All other args are passed down as `**kwargs` to `group`, so call out
which ones aren't.

Signed-off-by: Patrick Roy <[email protected]>
@roypat roypat force-pushed the buildkite-dependencies branch from 2c05520 to 353e8af Compare September 16, 2024 10:21
@roypat roypat marked this pull request as ready for review September 16, 2024 10:24
@roypat roypat added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Sep 16, 2024
@roypat roypat merged commit b7a1e9d into firecracker-microvm:main Sep 16, 2024
6 of 7 checks passed
roypat added a commit to roypat/firecracker that referenced this pull request Sep 16, 2024
@roypat
buildkite: drop explicit "depends_on": None step entries
17b53ce 
These were needed when the we still had a `wait` step (to overwrite the
`wait` step and instead start immediately), however since firecracker-microvm#4803 we do
not have a `wait` step anymore (instead everything that used to be after
the wait step now explicitly depends on the compilation steps). Thus,
having the `depends_on: null` entries for kani and style steps is no
longer needed (it also does not harm, but it looks ugly in buildkite's
canvas view).

Signed-off-by: Patrick Roy <[email protected]>
roypat added a commit that referenced this pull request Sep 16, 2024
@roypat
buildkite: drop explicit "depends_on": None step entries
b99bfce 
These were needed when the we still had a `wait` step (to overwrite the
`wait` step and instead start immediately), however since #4803 we do
not have a `wait` step anymore (instead everything that used to be after
the wait step now explicitly depends on the compilation steps). Thus,
having the `depends_on: null` entries for kani and style steps is no
longer needed (it also does not harm, but it looks ugly in buildkite's
canvas view).

Signed-off-by: Patrick Roy <[email protected]>
RiverPhillips pushed a commit to RiverPhillips/firecracker that referenced this pull request Sep 20, 2024
@roypat @RiverPhillips
buildkite: drop explicit "depends_on": None step entries
aedeeac 
These were needed when the we still had a `wait` step (to overwrite the
`wait` step and instead start immediately), however since firecracker-microvm#4803 we do
not have a `wait` step anymore (instead everything that used to be after
the wait step now explicitly depends on the compilation steps). Thus,
having the `depends_on: null` entries for kani and style steps is no
longer needed (it also does not harm, but it looks ugly in buildkite's
canvas view).

Signed-off-by: Patrick Roy <[email protected]>
Signed-off-by: River Phillips <[email protected]>
ShadowCurse pushed a commit to ShadowCurse/firecracker that referenced this pull request Nov 5, 2024
@roypat @ShadowCurse
buildkite: drop explicit "depends_on": None step entries
f40905a 
These were needed when the we still had a `wait` step (to overwrite the
`wait` step and instead start immediately), however since firecracker-microvm#4803 we do
not have a `wait` step anymore (instead everything that used to be after
the wait step now explicitly depends on the compilation steps). Thus,
having the `depends_on: null` entries for kani and style steps is no
longer needed (it also does not harm, but it looks ugly in buildkite's
canvas view).

Signed-off-by: Patrick Roy <[email protected]>
ShadowCurse pushed a commit to ShadowCurse/firecracker that referenced this pull request Nov 5, 2024
@roypat @ShadowCurse
buildkite: drop explicit "depends_on": None step entries
ea16e36 
These were needed when the we still had a `wait` step (to overwrite the
`wait` step and instead start immediately), however since firecracker-microvm#4803 we do
not have a `wait` step anymore (instead everything that used to be after
the wait step now explicitly depends on the compilation steps). Thus,
having the `depends_on: null` entries for kani and style steps is no
longer needed (it also does not harm, but it looks ugly in buildkite's
canvas view).

Signed-off-by: Patrick Roy <[email protected]>
ShadowCurse pushed a commit to ShadowCurse/firecracker that referenced this pull request Nov 5, 2024
@roypat @ShadowCurse
buildkite: drop explicit "depends_on": None step entries
e95d448 
These were needed when the we still had a `wait` step (to overwrite the
`wait` step and instead start immediately), however since firecracker-microvm#4803 we do
not have a `wait` step anymore (instead everything that used to be after
the wait step now explicitly depends on the compilation steps). Thus,
having the `depends_on: null` entries for kani and style steps is no
longer needed (it also does not harm, but it looks ugly in buildkite's
canvas view).

Signed-off-by: Patrick Roy <[email protected]>
ShadowCurse pushed a commit to ShadowCurse/firecracker that referenced this pull request Nov 5, 2024
@roypat @ShadowCurse
buildkite: drop explicit "depends_on": None step entries
a7aa0a0 
These were needed when the we still had a `wait` step (to overwrite the
`wait` step and instead start immediately), however since firecracker-microvm#4803 we do
not have a `wait` step anymore (instead everything that used to be after
the wait step now explicitly depends on the compilation steps). Thus,
having the `depends_on: null` entries for kani and style steps is no
longer needed (it also does not harm, but it looks ugly in buildkite's
canvas view).

Signed-off-by: Patrick Roy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zulinx86 zulinx86 zulinx86 approved these changes

@pb8o pb8o pb8o approved these changes

Assignees

No one assigned

Labels

Status: Awaiting review Indicates that a pull request is ready to be reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@roypat @zulinx86 @pb8o